Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add primary key to MultiIndex index fn params #781

Merged
merged 5 commits into from
Aug 23, 2022
Merged

Add primary key to MultiIndex index fn params #781

merged 5 commits into from
Aug 23, 2022

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Aug 16, 2022 •
edited
Loading

Closes #670. Adds the (raw) primary key to the MultiIndex index fn params.

It is then the index function's responsibility to deserialise the pk, if it wants to use all or parts of it for building the index key.

TODO:

  • Add example / tests for this indexing mechanism. (done)
  • Add MIGRATING entry / notice. (done, for upcoming v0.14.0)

@maurolacy maurolacy requested a review from ueco-jb August 16, 2022 21:20
@maurolacy maurolacy self-assigned this Aug 16, 2022
@maurolacy maurolacy marked this pull request as draft August 16, 2022 21:21
@maurolacy maurolacy changed the title Add primary key to MultIndex index fn params Add primary key to MultiIndex index fn params Aug 16, 2022
@maurolacy maurolacy marked this pull request as ready for review August 17, 2022 09:05
@maurolacy maurolacy requested a review from ethanfrey August 17, 2022 09:06
@maurolacy maurolacy requested a review from uint August 17, 2022 09:06
@maurolacy maurolacy requested a review from hashedone August 17, 2022 09:07
hashedone
hashedone approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@hashedone hashedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@maurolacy maurolacy merged commit a7eb8f4 into main Aug 23, 2022
@maurolacy maurolacy deleted the 670-multindex-index-fn branch August 23, 2022 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashedone hashedone hashedone approved these changes

@ueco-jb ueco-jb Awaiting requested review from ueco-jb

@ethanfrey ethanfrey Awaiting requested review from ethanfrey

@uint uint Awaiting requested review from uint

Assignees

@maurolacy maurolacy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change MultiIndex index function signature to include the pk
2 participants
@maurolacy @hashedone